Fix dead code and logic error #8714
Conversation
1. added missing SPADE test cases to LATENT_CNDM_TEST_CASES 2. fixed the SPADE branch in test_sample_intermediates Signed-off-by: ytl0623 <david89062388@gmail.com>
📝 WalkthroughWalkthroughAdded SPADEAutoencoderKL as a new latent autoencoder variant in latent diffusion inference tests. The LATENT_CNDM_TEST_CASES_DIFF_SHAPES matrix now includes a SPADEAutoencoderKL configuration paired with SPADEDiffusionModelUNet, conditioning embedding, input/output latent shapes, and label_nc. Tests were updated to construct SPADEAutoencoderKL where ae_model_type matches, adjust segmentation input shapes to label_nc, include SPADEAutoencoderKL in intermediate-enabled branches (save_intermediates, intermediate_steps), and route its intermediates through the existing inferer pathways used for SPADEDiffusionModelUNet. Numerical assertion style changed to use assert_close for a normal CDF comparison. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: ytl0623 <david89062388@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/inferers/test_controlnet_inferers.py (1)
851-860: Fix stage_2 selection intest_pred_shape.
Theelsecurrently binds to the SPADEAutoencoderKL check and can overwrite a SPADEDiffusionModelUNet selection.🔧 Proposed fix
- if dm_model_type == "SPADEDiffusionModelUNet": - stage_2 = SPADEDiffusionModelUNet(**stage_2_params) - if ae_model_type == "SPADEAutoencoderKL": - stage_1 = SPADEAutoencoderKL(**autoencoder_params) - else: - stage_2 = DiffusionModelUNet(**stage_2_params) + if dm_model_type == "SPADEDiffusionModelUNet": + stage_2 = SPADEDiffusionModelUNet(**stage_2_params) + else: + stage_2 = DiffusionModelUNet(**stage_2_params) + if ae_model_type == "SPADEAutoencoderKL": + stage_1 = SPADEAutoencoderKL(**autoencoder_params)
Signed-off-by: ytl0623 <david89062388@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/inferers/test_controlnet_inferers.py (1)
849-860: Missing autoencoder instantiation for non-SPADE cases.
stage_1remainsNoneforAutoencoderKLandVQVAEtest cases. Line 860 will raiseAttributeError.🐛 Proposed fix
stage_1 = None if dm_model_type == "SPADEDiffusionModelUNet": stage_2 = SPADEDiffusionModelUNet(**stage_2_params) else: stage_2 = DiffusionModelUNet(**stage_2_params) + if ae_model_type == "AutoencoderKL": + stage_1 = AutoencoderKL(**autoencoder_params) + if ae_model_type == "VQVAE": + stage_1 = VQVAE(**autoencoder_params) if ae_model_type == "SPADEAutoencoderKL": stage_1 = SPADEAutoencoderKL(**autoencoder_params) controlnet = ControlNet(**controlnet_params)
Signed-off-by: ytl0623 <david89062388@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/inferers/test_controlnet_inferers.py (1)
1334-1360: Bug: Missingsegfor SPADEAutoencoderKL with regular DiffusionModelUNet.The test case at lines 362-401 pairs
SPADEAutoencoderKLwithDiffusionModelUNet. Line 1334 only checks forSPADEDiffusionModelUNet, so this case takes the else branch and doesn't passseg.SPADEAutoencoderKL.decode_stage_2_outputs()requiresseg.🐛 Proposed fix
- if dm_model_type == "SPADEDiffusionModelUNet": + if dm_model_type == "SPADEDiffusionModelUNet" or ae_model_type == "SPADEAutoencoderKL":
🧹 Nitpick comments (4)
tests/inferers/test_controlnet_inferers.py (4)
1017-1043: Inconsistent SPADE handling intest_get_likelihoods.The condition at line 1017 only checks for
SPADEDiffusionModelUNet, buttest_prediction_shape(line 808) and other tests check for both SPADE variants. If a future test case usesSPADEAutoencoderKLwith a regularDiffusionModelUNet, this test will fail becausesegwon't be passed to the inferer.Consider aligning this condition:
- if dm_model_type == "SPADEDiffusionModelUNet": + if ae_model_type == "SPADEAutoencoderKL" or dm_model_type == "SPADEDiffusionModelUNet":
1087-1115: Same inconsistency astest_get_likelihoods.Line 1087 should also check for
SPADEAutoencoderKLto stay consistent with other tests.
1171-1201: Same inconsistency as above.Line 1171 should also check for
SPADEAutoencoderKL.
1253-1281: Same inconsistency as above.Line 1253 should also check for
SPADEAutoencoderKL.
Signed-off-by: ytl0623 <david89062388@gmail.com>
Since PyTorch 1.10, assert_allclose is deprecated. This change migrates to assert_close and explicitly converts inputs to tensors to satisfy stricter type checks. Signed-off-by: ytl0623 <david89062388@gmail.com>
|
I've fixed the bugs, thanks! |
2 participants
Description
SPADEtest cases toLATENT_CNDM_TEST_CASESSPADEbranch intest_sample_intermediatesTypes of changes
./runtests.sh -f -u --net --coverage../runtests.sh --quick --unittests --disttests.make htmlcommand in thedocs/folder.